Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Draft] Group docker simple flow tests to suite #8393

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[Draft] Group docker simple flow tests to suite #8393

wants to merge 1 commit into from

Conversation

d8660091
Copy link
Member

Issue #, if available:

Description of changes:

  1. Convert Docker SimpleFlow to subtests, removed duplicated code
  2. Quick e2e test regexps are now passing as a new flag, to make it allow to pick an subtest inside the TestSuite

Testing (if applicable): Manually invoked the quick-e2e-tests with this code change

Documentation added/planned (if applicable):

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@eks-distro-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from d8660091. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@eks-distro-bot eks-distro-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 26, 2024
@d8660091 d8660091 force-pushed the mt branch 5 times, most recently from 6c98737 to ea5a726 Compare June 26, 2024 16:17
@codecov Codecov
Copy link

codecov bot commented Jun 26, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.48%. Comparing base (b399c50) to head (8e7ccd1).
Report is 13 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #8393   +/-   ##
=======================================
  Coverage   73.48%   73.48%           
=======================================
  Files         578      578           
  Lines       36489    36489           
=======================================
  Hits        26814    26814           
  Misses       7956     7956           
  Partials     1719     1719

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jaxesn
jaxesn reviewed Jun 27, 2024
View reviewed changes
test/e2e/docker_test.go
@@ -1480,3 +1434,16 @@ func TestDockerKubernetes130KubeletConfigurationSimpleFlow(t *testing.T) {
)
runKubeletConfigurationFlow(test)
}

func TestDockerKubernetesSimpleFlowSuite(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to see a mock of this method that handles all OSs and Providers, you dont have to make it work out of the gate, but Id like to see it so we can iterate on the design here before rolling this out.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Guillermo has made one showing how the suite handles OSs and Providers: https://github.com/aws/eks-anywhere/compare/main...g-gaston:eks-anywhere:poc-multi-test?expand=1

Copy link
Member Author

@d8660091 d8660091 Jun 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a gut feeling, parameterizing provider could make the e2e test harder to write and maintain, which should be as simple as a test yaml file + "eksa create cluster". I am OK with parameterize OS though

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jaxesn jaxesn jaxesn left review comments

Assignees
No one assigned
Labels
do-not-merge/hold size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@d8660091 @eks-distro-bot @jaxesn